Fix/wallet fee calculation #1916
Conversation
OBorce
force-pushed
the
fix/wallet-fee-calculation
branch
from
05abe2e to
6b1fe65
Compare
- account for compact input length encoding - fix rounding error in case of individual input fee calculation vs total fee calculation using entire tx size - add missing limit for tx size/weight
OBorce
force-pushed
the
fix/wallet-fee-calculation
branch
from
6b1fe65 to
5a4271e
Compare
| let selection_result = select_coins( | ||
| utxos, | ||
| (amount_to_be_paid_in_currency_with_fees - preselected_amount).unwrap_or(Amount::ZERO), | ||
| PayFee::PayFeeWithThisCurrency, | ||
| cost_of_change, | ||
| selection_algo, | ||
| self.chain_config.max_tx_size_for_mempool(), | ||
| )?; |
There was a problem hiding this comment.
I wonder, why do we need to call select_coins in the case when amount_to_be_paid_in_currency_with_fees == preselected_amount?
Also, select_coins has this code:
if selection_target == Amount::ZERO && pay_fees == PayFee::DoNotPayFeeWithThisCurrency {
return Ok(SelectionResult::new(selection_target));
}
Why doesn't it ignore zero amount in the PayFeeWithThisCurrency case too?
It looks like we select coins for the change even when we don't need any change.
There was a problem hiding this comment.
removed, and fixed some tests that create a delegationId with 0 fee, where the algo would pick now inputs and the tx will fail. I added some small change outputs along the create delegation output to force selecting an input.
| (inputs.amount, inputs.fee, inputs.total_input_size) | ||
| }); | ||
|
|
||
| let (size_of_change, cost_of_change) = output_change_size_and_fees( |
There was a problem hiding this comment.
There is a tight coupling between output_change_size_and_fees and check_outputs_and_add_change - the former accepts Option<&Address<Destination>> and assumes PublicKeyHash if it's None. And the latter then calls key_chain.next_unused_address if change_addresses doesn't contain the corresponding currency, and next_unused_address is assumed to always return PublicKeyHash I guess, which is not a great assumption either.
I wonder if we could refactor it in this PR. E.g. modify change_addresses in advance, calling key_chain.next_unused_address if necessary, so that the map already contains all possible currencies.
(If not, then it's better to at least add some comments to output_change_size_and_fees and check_outputs_and_add_change, mentioning this interdependency).
There was a problem hiding this comment.
fixed and also moved creation of the change output right after the selection of the inputs to use the specific destination (preselected or next_unued_address).
wallet/src/account/mod.rs
Outdated
| // increase fee and total_input_size, this is same for all inputs | ||
| match preselected_inputs.entry(Currency::Coin) { | ||
| Entry::Vacant(entry) => { | ||
| entry.insert(PreselectedInputAmounts { | ||
| amount: Amount::ZERO, | ||
| fee, | ||
| burn: Amount::ZERO, | ||
| total_input_size, | ||
| }); | ||
| } | ||
| Entry::Occupied(mut entry) => { | ||
| let existing = entry.get_mut(); | ||
| existing.fee = (existing.fee + fee).ok_or(WalletError::OutputAmountOverflow)?; | ||
| existing.total_input_size += total_input_size; | ||
| } | ||
| }; |
There was a problem hiding this comment.
This looks hacky.
select_inputs_for_send_requestoperates onpay_fee_with_currency, so I guess you should pass it here instead of assuming that it's always Coin.
(or if you think it's redundant, then maybeselect_inputs_for_send_requestshould no longer pretend that it can handle other currencies to pay fees with).- The code in
select_inputs_for_send_request, where it iterates over "non-pay_fee_with_currency" currencies and updates the total tx size and fee, doesn't make sense anymore, because those values will always be zeroes now.
So,
a) I wonder what was wrong with the previous approach. Why can't we return the sizes/fees on per-currency basis as before, and make the caller combine the result (like it still does)?
b) If the old approach is not good anymore, then maybe this function's return type should be changed to something else that better reflects the data that is being collected?
There was a problem hiding this comment.
removed those fields from the PreselectedInputAmounts, and created a new struct as the size and fees are unrelated to the currency.
OBorce
force-pushed
the
fix/wallet-fee-calculation
branch
2 times, most recently
from
025bb56 to
8184900
Compare
OBorce
force-pushed
the
fix/wallet-fee-calculation
branch
from
8184900 to
be73675
Compare
This was fixed by passing the number of inputs to the
tx_size_with_outputs.Closes #1208